Allow HomeKit name to be customized by schmittx · Pull Request #14159 · home-assistant/core

schmittx · 2018-04-29T03:24:25Z

Description:

HomeKit accessory information can be customized by specifying name within entity_config.

Related issue (if applicable): N/A

Pull request in home-assistant.github.io with documentation (if applicable): home-assistant/home-assistant.io#5314

Example entry for `configuration.yaml` (if applicable):

homekit:
  port: 51825
  filter:
    include_domains:
      - lock
  entity_config:
    lock.front_door:
      name: Front Door Lock

Checklist:

The code change is tested and works locally.
Local tests pass with tox. Your PR cannot be merged unless tests pass

If user exposed functionality or configuration variables are added/changed:

Documentation added/updated in home-assistant.github.io

If the code communicates with devices, web services, or third-party tools:

New dependencies have been added to the REQUIREMENTS variable (example).
New dependencies are only imported inside functions that use them (example).
New dependencies have been added to requirements_all.txt by running script/gen_requirements_all.py.
New files were added to .coveragerc.

If the code does not interact with devices:

Tests have been added to verify that the new code works.

schmittx · 2018-04-29T03:35:10Z

@cdce8p This is my first draft, take a look and let me know your feedback. As discussed in #14114, this doesn't provide much direct functionality but just gives a cleaner experience for those that care. It's a similar concept to that of the Alexa cloud component.

cdce8p · 2018-04-29T08:07:47Z

I have to think about it, but at the moment I'm not convinced this should be added.

cdce8p · 2018-05-04T21:28:58Z

There have been a few changes with #14278
If you rebase it, I can take a new look at it next week

schmittx · 2018-05-04T23:43:40Z

@cdce8p Rebased with the latest updates

cdce8p

Although I personally don't see much value in it, we can move forward with it.
Can you add tests and update the doc?

cdce8p · 2018-05-05T03:02:43Z

Move that call to get_accessory

cdce8p · 2018-05-05T09:05:39Z

The empty lines between these four config opions (name - serial_number) can be removed

cdce8p · 2018-05-05T09:10:01Z

Regarding the config dictionary: I was thinking about how we can improve it since I don't
Iike adding it to every super call.

Why don't we use the same custruct as for hass and the entity_id? Remove the keyword in get_accessory and pass it through args. Than assign it to self.config in HomeAccessory.

schmittx · 2018-05-06T05:02:11Z

@cdce8p I updated per your comments, updated the doc, and updated test_util.py. I'm not sure what other tests need to be added or updated, can you give a little advice?

cdce8p · 2018-05-06T08:28:19Z

@schmittx As a starting point take a look at the failing tests: https://travis-ci.org/home-assistant/home-assistant/jobs/375445342#L808. Since config is no longer a keyword argument, most of the accessory tests are failing. As an example:

acc = GarageDoorOpener(self.hass, 'Cover', garage_door, 2, config=None)

# Would need to change to this:
acc = GarageDoorOpener(self.hass, 'Cover', garage_door, 2)

Basically config can be removed, except for security_systems.

Another thing would be to add checks for the newly added vars to test_home_accessory: https://github.com/home-assistant/home-assistant/blob/dev/tests/components/homekit/test_accessories.py#L91-L97

And for the name config var a test in get_accessories, here: https://github.com/home-assistant/home-assistant/blob/dev/tests/components/homekit/test_get_accessories.py#L33

schmittx · 2018-05-06T18:32:38Z

@cdce8p Thanks for the help, I updated/added most of the tests without problem. But, I keep getting a KeyError with test_get_accessory_customize_name and test_alarm_control_panel as they're currently written. Can you take a look and give me some suggestions?

cdce8p

I added a few comments that should resolve the failing tests.

Although my initial doubts, this is coming together quite nicely.
Just a few notes FYI:

Most likely a PR will be merged soon that changes one of the homekit tests, which will require a small rebase #14314
Even after all changes I would like to wait a few more days (the next weekend, maybe earlier) before I merge this. That way I'm still able to push bug fixes if I need to.

cdce8p · 2018-05-06T20:42:49Z

Can you change the name to test_customize_name? That should be enough.
Also for the first test test_invalid_aid might be better as well. Just noticed it.

I agree, I cleaned up both names.

cdce8p · 2018-05-06T20:45:41Z

Regarding the actual test:
The type modules are imported dynamically, but not for this test module. Therefore the Light type has to be mocked. The following should work:

def test_customize_name(): """Test with customized name.""" mock_type = Mock() with patch.dict(TYPES, {'Light': mock_type}): config = {CONF_NAME: 'Customize Name'} acc = get_accessory(None, State('light.demo', 'on'), 2, config) mock_type.assert_called_with( None, 'Customize Name', 'light.demo', 2, config)

Instead of validation the the display_name is set correctly, we just check that the right name was passed.

For the alarm_control_panel test:
If you replace this lines: https://github.com/schmittx/home-assistant/blob/b41effa8fad337c7868e1e831835e34420a18916/tests/components/homekit/test_get_accessories.py#L173-L175 with:

self.assertEqual(self.mock_type.call_args[0][-1][ATTR_CODE], '1234')

that should solve it.

The reason for this failure was the change from a keyword to a normal argument.

Awesome, thanks for the assist!

cdce8p · 2018-05-06T21:00:32Z

What do you think of:

for key in (CONF_NAME, CONF_MANUFACTURER, CONF_MODEL, CONF_SERIAL_NUMBER): value = config.get(key) if value: params[key] = value

That way we only add dict entries if a value exists. It also fixes test_validate_entity_config.

Agreed, much cleaner.

cdce8p · 2018-05-06T21:02:58Z

Instead of initializing a new HomeAccessory, just combine it with the previous one.

schmittx · 2018-05-06T21:44:56Z

@cdce8p Sounds good to me, this should be good to go now once the tests pass.

FYI, I'm also working on adding support for Fan entities. I should have that PR ready later this week or next weekend.

cdce8p · 2018-05-06T23:43:53Z

I would prefer to use key instead.. index is usually used for an int counter through lists, so it might lead to confusion here.

key is already used in the top level at line 20, so I went with char in reference to the fact that they're used for characteristic customize.

Missed that. What do you think of changing the top level one to entity_id and using key here? (At least if I haven't missed something again.)

cdce8p · 2018-05-06T23:45:38Z

I just realized that we wouldn't/can't cover this case otherwise with the accessory below.
We might have to add it back. Sorry for the confusion.

Added back and cleaned up test_home_accessory, hopefully you agree with my changes.

cdce8p · 2018-05-06T23:52:49Z

@schmittx Regarding fan support. I think someone else was already working on it, although I haven't seen an open PR yet. Take a look at #13805.

We should't talk to much about it in this PR though, to keep it clean. Find me on discord.

schmittx · 2018-05-08T02:33:41Z

@cdce8p I read through #14330 as well as the repos that it mentioned. I understand the benefit of being able to see the history for accessories and persistent storage but I'd be hesitant to implement it due to the convoluted method required and the need to use the Eve app exclusively (if history is desired). I also think there's a high risk of regular maintenance being needed in the event that Elgato changes their services/characteristics.

I think we should focus on using the native Apple HomeKit services/characteristics in order to guarantee a consistent experience for all users, regardless of what app is used (Home, Eve, etc.).

For now, I'd vote to focus on improving the current HomeKit component (i.e. add support for more entities like media_player or show switches as valve/sprinklers, adding optional characteristics to existing entities, battery tracking, etc.).

If we run out of things to improve for this component and want to revisit Fakegato in the future then let's worry about the accessory information at that time (Maybe we'd make Fakegato a config option, which would then overwrite any accessory information with unique serial numbers, etc.?)

cdce8p · 2018-05-08T10:31:36Z

I agree with almost all of your points. The native services and chars are just easier to maintain at the moment and the focus should be an improving support for other components (eg. Media Player, Fan, ...).

What I'm worried about however is that it's quite difficult to take a feature away once it's implemented. If we really decide to implement fakegate, it shouldn't come at the cost of overriding the serial number if it's activated (maybe through a config option).

Therefore I would prefer if we limit the configurable parameters to the name only and assign the others as explained before. We can always add features later, but removing them is really difficult.

schmittx · 2018-05-08T11:32:10Z

I agree that it's difficult to remove a feature once it's implemented, I just struggle with limiting functionality now in the hopes that we might add something else in future.

I'll defer to you in the end though. If we limit the confutable parameters to just name, then I'd propose to close this PR and create a new one. Then we can always come back to this in the future if necessary.

cdce8p · 2018-05-08T13:35:42Z

I agree that it's difficult to remove a feature once it's implemented, I just struggle with limiting functionality now in the hopes that we might add something else in future.

Take in mind that this doesn't add any new functionality, just the option to customize some additional parts.

If we limit the confutable parameters to just name, then I'd propose to close this PR and create a new one. Then we can always come back to this in the future if necessary.

We shouldn't do that. This PR has some great foundations in place to implement this feature. A new PR would just copy most of it. A better way might be to create a new PR once this is merge that adds the additional parameters as well and close that for the time being.

cdce8p · 2018-05-10T23:30:25Z

Can you change key to entity_id in line 19 and 20, the for loop? That way we won't have this problem in the future.

No problem, done.

schmittx · 2018-05-11T00:07:23Z

@cdce8p OK, everything should be good now. Rebased with #14377 and a small cleanup afterwards.

cdce8p · 2018-05-11T00:33:19Z

@schmittx Will take a closer look tomorrow. Thanks for taking the time!

cdce8p · 2018-05-11T00:52:02Z

@schmittx BTW: I have started looking into the complete async conversion for the homekit component. homekit-async if you want to take a look.

Are you in the HA discord channel? Discussing it there or in direct msg might be the easiest.

schmittx requested a review from cdce8p as a code owner April 29, 2018 03:24

homeassistant added cla-signed integration: homekit labels Apr 29, 2018

cdce8p self-assigned this Apr 30, 2018

schmittx closed this May 4, 2018

schmittx force-pushed the homekit-customize-accessory-information branch from 2509155 to 255a85a Compare May 4, 2018 23:21

homeassistant added the cla-signed label May 4, 2018

schmittx reopened this May 4, 2018

homeassistant added the cla-signed label May 4, 2018

cdce8p requested changes May 5, 2018

View reviewed changes

Comment thread homeassistant/components/homekit/accessories.py Outdated

Copy link
Copy Markdown

Member

cdce8p May 5, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Move that call to get_accessory

cdce8p reviewed May 5, 2018

View reviewed changes

cdce8p added the new-feature label May 5, 2018

homeassistant added the cla-signed label May 5, 2018

schmittx mentioned this pull request May 6, 2018

Allow HomeKit name to be customized home-assistant/home-assistant.io#5314

Merged

2 tasks

homeassistant added the cla-signed label May 6, 2018

cdce8p reviewed May 6, 2018

View reviewed changes

homeassistant added the cla-signed label May 6, 2018

schmittx changed the title ~~[WIP] Allow HomeKit accessory information to be customized~~ Allow HomeKit accessory information to be customized May 6, 2018

cdce8p reviewed May 6, 2018

View reviewed changes

cdce8p mentioned this pull request May 10, 2018

Add HomeKit support for fans #14351

Merged

8 tasks

homeassistant added the cla-signed label May 10, 2018

cdce8p mentioned this pull request May 10, 2018

Rewritten HomeKit tests #14377

Merged

2 tasks

schmittx added 3 commits May 10, 2018 19:24

Initial commit

3e1035b

Update per comments

84bdb55

Add util test

6b17282

cdce8p reviewed May 10, 2018

View reviewed changes

schmittx added 4 commits May 10, 2018 19:47

Update accessory tests

8b4b4cc

Updated tests

ebd829f

Cleaned up tests and minor change to util

124f5eb

Remove manufacturer, model, and serial number customize

040d56b

schmittx force-pushed the homekit-customize-accessory-information branch from ba7c62d to 040d56b Compare May 11, 2018 00:04

homeassistant added the cla-signed label May 11, 2018

Fix tests and small change per comment

41cd8db

schmittx changed the title ~~Allow HomeKit accessory information to be customized~~ Allow HomeKit name to be customized May 11, 2018

Small changes

bab1c5b

homeassistant added the cla-signed label May 11, 2018

cdce8p approved these changes May 11, 2018

View reviewed changes

cdce8p merged commit 621c653 into home-assistant:dev May 11, 2018

balloob mentioned this pull request May 28, 2018

0.70.0 #14657

Merged

schmittx deleted the homekit-customize-accessory-information branch June 2, 2018 00:04

girlpunk pushed a commit to girlpunk/home-assistant that referenced this pull request Sep 4, 2018

Allow HomeKit name to be customized (home-assistant#14159)

725e1d0

home-assistant locked and limited conversation to collaborators Sep 5, 2018

Uh oh!

Conversation

schmittx commented Apr 29, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description:

Example entry for configuration.yaml (if applicable):

Checklist:

Uh oh!

schmittx commented Apr 29, 2018

Uh oh!

cdce8p commented Apr 29, 2018

Uh oh!

cdce8p commented May 4, 2018

Uh oh!

schmittx commented May 4, 2018

Uh oh!

cdce8p left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cdce8p commented May 5, 2018

Uh oh!

schmittx commented May 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cdce8p commented May 6, 2018

Uh oh!

schmittx commented May 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cdce8p left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

schmittx commented May 6, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

schmittx May 7, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cdce8p May 7, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cdce8p commented May 6, 2018

Uh oh!

schmittx commented May 8, 2018

Uh oh!

cdce8p commented May 8, 2018

Uh oh!

schmittx commented May 8, 2018

Uh oh!

cdce8p commented May 8, 2018

Uh oh!

schmittx commented Apr 29, 2018 •

edited

Loading

Example entry for `configuration.yaml` (if applicable):

schmittx commented May 6, 2018 •

edited

Loading

schmittx commented May 6, 2018 •

edited

Loading

schmittx May 7, 2018 •

edited

Loading

cdce8p May 7, 2018 •

edited

Loading